-
Notifications
You must be signed in to change notification settings - Fork 790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support for semver filter in sync command #2189
Conversation
49da4ab
to
4f614ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on!
Outstanding review comments: #1918 (review)
(Also, eventually please squash the “formate sync.go file” commit, so that the code passes tests on every commit.)
dfc22f7
to
631cee2
Compare
@mtrmac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
(Eventually a rebase&squash would be preferred over intermediate commits and merges, but that’s non-blocking)
RPC failed; HTTP 500 curl 22 The requested URL returned error: 500
Let’s see if a future version passes; the macOS failures at that stage are certainly unrelated.
I now have problem: when I run the sync command, it prints "Start filtering using the regular expression:" log message multiple times @mtrmac I think we can delete this log message because the code will need extra unnecessary changes if we want to keep it |
Looking forward to getting this PR merged. Do @husseinferas or @mtrmac have any opinions on what I posted in #1918 (comment) ? From long-term experience I can conclude that:
My gut feeling is that now would be the best time to solve that problem, as otherwise breaking changes might be inevitable. |
Thank you @MShekow |
I fully agree with you to keep PRs (which implement things) small. What I'm suggesting is merely that we should already brainstorm whether any potential syntax changes (in the input yaml file) would be good to introduce now. |
@MShekow Looking at your proposal, it does seem to be compatible with the proposed structure, right now. Is there anything that you think needs changing? |
To reiterate, the currently proposed structure (that is also implemented in this PR) is something like this: images-by-semver:
alpine:
- "3.12 - 3.13" I can't think of a "good" solution for adding the prefilter-regex (that contains the named capture group) that I suggested in #1918 (comment). You would have to use funky syntax, mangling the prefilter-regex into the version string somehow, e.g. by replacing the version string (here: That was the entire reason why I proposed to add an additional YAML "layer" in #1918 (comment) that separates the list of semver-strings from the list of prefilter-regexes. |
good, so our current structure will work just fine in future with regex layer
with regex, in next implementation
|
Code LGTM |
Thank you @TomSweeneyRedHat can you approve the PR? |
@husseinferas, the branch needs updating. I've just kicked that off. Once completed, I'd like @mtrmac to do the final approval to ensure his comments were addressed. |
Oh… now I get what you mean. I definitely agree that a separate field is better than trying to stuff things inside the server filter. OTOH it’s a bit tedious to force every user to add one more level of nesting for a hopefully fairly rare case. Instead of registry.example.com:
images-by-semver:
alpine:
regex-filters: "…"
versions: ">= 3.12.0" we can do registry.example.com:
images-by-semver:
alpine: ">= 3.12.0"
semver-regex-filters:
alpine: "…" and only the repos which need that filter would pay the price. (Really long-term we should probably end up with sync:
- source: registry.source.com/source-repo
destination: registry.source.com/dest-repo
tags: …
regex: …
semver: …
semver-regex-filter: … but that’s a long-term future direction.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM; please squash&rebase into a single commit.
c8264af
to
db8f93e
Compare
use single semver constraint Signed-off-by: Hussein Firas <[email protected]> Signed-off-by: Miloslav Trmač <[email protected]>
db8f93e
to
177d4ad
Compare
(For transparency, I have rebased the work, to avoid WIP commits; per https://github.com/containers/skopeo/compare/db8f93e33ea3b99b157ef0c74226c922d7ecd658..177d4adb205dffa0e0e50d0709ee27b953ee0c00 the code is identical.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
I'm super happy that this PR has come to a successful conclusion :). @husseinferas are you interested in implementing the regex filter I mentioned (using the proposal made in #2189 (comment)), to increase the usefulness of this PR's semver filter? If not, I would have a go at it, it might take a while, however. |
Thanks @MShekow @mtrmac I think we need to update the sync YAML file structure for the next iteration because the current sync file is not scalable and has another problem that we need to fix, which is the destination repository mentioned here. sync:
- source: registry.source.com/source-repo
destination: registry.source.com/dest-repo
tags: …
regex: …
semver: …
semver-regex-filter: … I am willing to work on this feature, @mtrmac How does the team communicate? Is there a Slack channel or chat group somewhere? I had many questions regarding this update. |
Congrats on the release in v1.15.0 🎉
Really looking forward to the next iteration. I suppose that we're waiting for input from @mtrmac ? |
In general there’s a long-term need to redesign the new YAML format; the GitHub repo contains quite a few open issues directly or indirectly touching on the needs and design requirements. I’m afraid it’s also not a feature I’ve not been able to invest significant time in. These GitHub issues are the primary collaboration mechanism of the project. |
Adding Kubasobon's changes in the PR and rebase it with main to move it forward
more changes added: